Remove schema data #343
Remove schema data #343
Conversation
benjeffery
force-pushed
the
remove-schema-data
branch
from
9f727a1 to
f3c040d
Compare
|
Yes, looks good. There's a few details to follow up on, but it's the right approach. |
benjeffery
force-pushed
the
remove-schema-data
branch
from
f3c040d to
d61af16
Compare
benjeffery
force-pushed
the
remove-schema-data
branch
2 times, most recently
from
d30687c to
5ce3baa
Compare
|
Rebased |
jeromekelleher
left a comment
jeromekelleher
left a comment
There was a problem hiding this comment.
Looks good, I think we just need to formalise the interface we're implementing a bit here.
CHANGELOG.md
Outdated
|
|
||
| - Add support for unindexed (and uncompressed) VCFs (#337) | ||
|
|
||
| - Remove explicit sample, contig and filter lists from the schema (#343) |
There was a problem hiding this comment.
This is a breaking schema format change, so will require existing ICFs to be recreated. I listed these as "Breaking changes" below
bio2zarr/vcz.py
Outdated
| self.encode_samples(root) | ||
| self.encode_filter_id(root) | ||
| self.encode_contig_id(root) | ||
| if hasattr(self.source, "samples"): |
There was a problem hiding this comment.
Control flow by hasattr is pretty :vomit: for me - so easy for mistakes to get hidden as code evolves over time.
Every implementation has to have samples anyway, right? And, I would imagine likewise for contigs, so they should both be unconditional.
For filters, I think we should probably define a superclass which defines the interface we require here, and have filters return None by default. That way we can detect if filters (and other optional stuff) makes sense or not.
benjeffery
force-pushed
the
remove-schema-data
branch
2 times, most recently
from
de7b6ce to
5f1f224
Compare
|
I've added a base class for the sources and removed the mapping base class from the icf source. I've kept contigs optional as plink doesn't have them in this PR. |
benjeffery
force-pushed
the
remove-schema-data
branch
from
5f1f224 to
aedf90c
Compare
3 participants
Stacked on #339 - clean diff at 9f727a1